Add `context` attribute to link `TODO`s to GitHub issues #107

couimet · 2025-10-23T18:27:23Z

This PR introduces an optional context attribute for TODO comments that allows linking them to GitHub issues. This provides better traceability between code TODOs and their corresponding GitHub issues.

Key Features

New context attribute: Can be added to any TODO with an event trigger
Rich context information: When a TODO is triggered, the linked issue's title, state, and assignee are included in the notification
Works with all events: Compatible with date, gem_release, gem_bump, and ruby_version events -- and issue_close/pull_request_close too even if they already reference issues/PRs because the context could provide more info

Example Usage

# TODO(on: date('2025-02-01'), to: '[email protected]', context:'shopify/smart_todo/108')
#   Implement the caching strategy discussed in the issue

Implementation Details

Context validation follows the same pattern as existing event validations
The smart_todo_cop validates context syntax and arguments
Context information is fetched only when the TODO event is triggered
If the issue cannot be fetched, the TODO notification is still sent without context

Closes #108

🤖 Generated with Claude Code

This PR introduces an optional `context` attribute for TODO comments that allows linking them to GitHub issues using the `issue()` function. This provides better traceability between code `TODO`s and their corresponding GitHub issues. ## Key Features - **New `context` attribute**: Can be added to any `TODO` with an event trigger - **`issue()` function**: Takes 3 parameters (org, repo, issue_number) - **Rich context information**: When a `TODO` is triggered, the linked issue's title, state, and assignee are included in the notification - **Works with all events**: Compatible with `date`, `gem_release`, `gem_bump`, and `ruby_version` events (not with `issue_close`/`pull_request_close` as they already reference issues/PRs) ## Example Usage ```ruby # TODO(on: date('2025-02-01'), to: '[email protected]', context: issue('shopify', 'smart_todo', '108')) # Implement the caching strategy discussed in the issue ``` ## Implementation Details - Context validation follows the same pattern as existing event validations - The smart_todo_cop validates context syntax and arguments - Context information is fetched only when the `TODO` event is triggered - If the issue cannot be fetched, the `TODO` notification is still sent without context Closes #108 🤖 Generated with Claude Code

…nsidered valid with `issue_close` and `pull_request_close`

lib/smart_todo/cli.rb

lib/smart_todo/todo.rb

lib/smart_todo_cop.rb

test/smart_todo/integration_test.rb

Edouard-chin

I think this is a great addition, but I have the feeling that this may try to over-engineer a solution. It also makes a todo definition very verbose and it becomes hard to read when looking at the TODO definition.

I wonder if a more simpler approach à la GitHub would be easier.

# TODO(on...)
#   Part of the bigger cleanup
#   Ref Shopify/super_repo#34

When SmartTodo detects the Ref ... part, it would transfom it to a link to be sent along the slack message

Co-authored-by: Rafael Mendonça França <[email protected]>

Ref: #107 (comment) and #107 (comment)

At the same time, I refactored to avoid code duplication and added tests Ref: #107 (comment)

couimet · 2025-10-24T12:35:22Z

I think this is a great addition, but I have the feeling that this may try to over-engineer a solution. It also makes a todo definition very verbose and it becomes hard to read when looking at the TODO definition.

I wonder if a more simpler approach à la GitHub would be easier.
# TODO(on...)
#   Part of the bigger cleanup
#   Ref Shopify/super_repo#34
When SmartTodo detects the Ref ... part, it would transfom it to a link to be sent along the slack message

Thanks for the feedback !

Using Ref <org>/<repo>/<id> format could indeed have been used, but I didn't come across it much in shop-server (the main repo I work on).

Given context is optional, people will be free to either use it or don't if they prefer to use a full link (or short Ref <org>/<repo>/<id> version) in a free-fom way in the comments.

I believe it's better to build context as structured info so it can easily be handled in Rubocop/code and additional features can be built on top of it if desired.

What do you think ?

lib/smart_todo/cli.rb

test/smart_todo/todo_test.rb

Edouard-chin · 2025-10-24T15:20:38Z

I understand that building a structured context makes it easier for us to parse. But it comes with the downside of making it harder for humans to write and read.

Writing a smart todo is already not the most user friendly (especially since its just a comment without any syntax highlighting), and I'm worried that adding a new programming syntax info to the TODO will clutter it even more.

Ultimately a TODO should be written to be read by humans not machines which is why I suggested the standard Ref repo/org#123. I honestly wouldn't bother reading the TODO metadata if I were to stumbled upon this:

# TODO(on: date('2025-03-15'), to: '#proj-slack-channel-session-something', context: issue('shopify', tapioca, 27721))

I wouldn't want to dismiss the great work you already put in this PR and since Rafael is also ok with this approach I'll defer to his approval. At the very minimum if we go with the structured context, please consider context: "org/repo#123 instead of context: issue('org', 'repo', 123) (which also opens the door for other context and more complexity)

rafaelfranca · 2025-10-24T17:29:55Z

I actually really like context: "org/repo#123" good idea. Can we see how supporting only that would look like?

…e complex construct that makes it harder for humans to process Refs: #107 (comment) and #107 (comment)

couimet · 2025-10-28T12:40:06Z

I actually really like context: "org/repo#123" good idea. Can we see how supporting only that would look like?

@rafaelfranca + @Edouard-chin I stacked #110 onto the current PR; for an easy diff of what it would look like.

Let me know what you think

Edouard-chin

Thanks for the stacked PR in #110 (it looks nice). I made a few of suggestions here.

lib/smart_todo/todo.rb

test/smart_todo/events/issue_context_test.rb

README.md

lib/smart_todo/events.rb

Add `context` attribute to link `TODO`s with GitHub issues (as string)

…`smart_todo` Ref: #107 (comment)

Ref: #107 (comment)

couimet · 2025-10-28T22:39:22Z

Thanks for the stacked PR in #110 (it looks nice). I made a few of suggestions here.

Addressed the received feedback; looks like the code is ready to be reviewed for a final pass (@Edouard-chin + @rafaelfranca ).

I kept ed3d9f8 and 77ec138 laser-focused on the PR.
If I properly interpreted the feedback in those 2 commits, I can circle back with a separate PR that brings the same improvements to main (for the rest of the already-existing codebase).

Edouard-chin

Thanks for your work on this 👍 . I just have one last feedback https://github.com/Shopify/smart_todo/pull/107/files#r2472368783

Ref: #107 (comment)

couimet · 2025-10-29T19:01:19Z

Thanks for your work on this 👍

Super excited to contribute to this public repo !

After merge, what action(s) do I need to take so a new release is issued and I can use this new format in other repos ?

Edouard-chin · 2025-10-29T22:56:47Z

You can cut a new release and deploy through our shipit stack to have the gem published on RubyGems. Shipit will take care of creating the git tag matching the bumped version.

You may also want to create a Github release and add a few lines of doc introduce in the new version.

Bring improvements from PR #107 I got interesting feedback for improvements in #107 In order to avoid feature-creep on my previous PR, I punted on touching files/methods outside the scope I was working on. This new PR brings improvements from ed3d9f8 and 77ec138 across the whole project. No functional change; simply adding consistency across files.

couimet · 2025-10-30T13:08:21Z

You can cut a new release and deploy through our shipit stack to have the gem published on RubyGems. Shipit will take care of creating the git tag matching the bumped version.

You may also want to create a Github release and add a few lines of doc introduce in the new version.

I couldn't find smart_todo in https://shipit-next.shopifycloud.com/

I ended up posting here: https://shopify.slack.com/archives/C2G36UR8B/p1761829568697519

couimet · 2025-10-30T13:35:47Z

I was pointed to https://shipit.shopify.io/shopify/smart_todo/rubygems to trigger the Deploy and https://rubygems.org/gems/smart_todo now features version 1.11.0

Edouard-chin · 2025-10-30T13:55:39Z

Awesome, thank you 👏.

Not a big deal, but for next time it would be preferable to squash your commits before merging for a cleaner git history.

couimet requested review from a team and rafaelfranca October 23, 2025 18:27

couimet changed the title ~~Add issue_pin event to link TODOs with GitHub issues~~ Add context attribute to link TODOs with GitHub issues Oct 23, 2025

Add missing validation and documentation to ensure context isn't co…

39ccf2b

…nsidered valid with `issue_close` and `pull_request_close`

couimet force-pushed the add-issue-pin-feature branch from 236b2b0 to 39ccf2b Compare October 23, 2025 21:06

couimet added 2 commits October 23, 2025 17:10

Fix broken tests

8abc253

Added missing newline

c9e8887

rafaelfranca reviewed Oct 23, 2025

View reviewed changes

Edouard-chin reviewed Oct 23, 2025

View reviewed changes

couimet and others added 3 commits October 23, 2025 18:18

Got rid of clanker-provided comments I should have removed

e66af5a

Co-authored-by: Rafael Mendonça França <[email protected]>

PR feedback: re-organize the code and remove some comments

532ecee

Ref: #107 (comment) and #107 (comment)

PR feedback: organize in a closer approach to other blocks above

2090a29

At the same time, I refactored to avoid code duplication and added tests Ref: #107 (comment)

couimet self-assigned this Oct 24, 2025

couimet commented Oct 24, 2025

View reviewed changes

lib/smart_todo/cli.rb Outdated Show resolved Hide resolved

Remove comment that belongs somewhere else

5d196bd

couimet commented Oct 24, 2025

View reviewed changes

test/smart_todo/todo_test.rb Outdated Show resolved Hide resolved

Add missing newline

7a0a3cf

couimet requested review from Edouard-chin and rafaelfranca October 24, 2025 12:40

PR feedback: use a string format for context instead of using a mor…

d030ac6

…e complex construct that makes it harder for humans to process Refs: #107 (comment) and #107 (comment)

couimet mentioned this pull request Oct 28, 2025

Add context attribute to link TODOs with GitHub issues (as string) #110

Merged

Make rubocop happy

6bd985a

Edouard-chin reviewed Oct 28, 2025

View reviewed changes

lib/smart_todo/todo.rb Outdated Show resolved Hide resolved

test/smart_todo/events/issue_context_test.rb Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

lib/smart_todo/events.rb Outdated Show resolved Hide resolved

Merge pull request #110 from Shopify/add_context_as_single_string

f6ad862

Add `context` attribute to link `TODO`s with GitHub issues (as string)

couimet added a commit that referenced this pull request Oct 28, 2025

PR feedback: remove restriction on context; it can be added to any …

93aed4e

…`smart_todo` Ref: #107 (comment)

couimet added 3 commits October 28, 2025 16:11

PR feedback: remove restriction on context; it can be added to any …

57477e8

…`smart_todo` Ref: #107 (comment)

PR feedback: use to_return_json instead of to_return + JSON.dump()

77ec138

Ref: #107 (comment)

PR feedback: rescuing for some errors and let the other ones go through

ed3d9f8

Ref: #107 (comment)

couimet requested a review from Edouard-chin October 28, 2025 22:39

Edouard-chin approved these changes Oct 29, 2025

View reviewed changes

couimet changed the title ~~Add context attribute to link TODOs with GitHub issues~~ Add context attribute to link TODOs to GitHub issues Oct 29, 2025

PR feedback: examples will be moved to the wiki

c8f0dca

Ref: #107 (comment)

couimet merged commit ced4c76 into main Oct 29, 2025
7 checks passed

couimet deleted the add-issue-pin-feature branch October 29, 2025 19:01

couimet mentioned this pull request Oct 29, 2025

Bring improvements from PR #107 to the rest of the project #111

Merged

shopify-shipit bot temporarily deployed to rubygems October 30, 2025 13:24 Inactive

Add context attribute to link TODOs to GitHub issues #107

Add context attribute to link TODOs to GitHub issues #107

Uh oh!

Conversation

couimet commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Features

Example Usage

Implementation Details

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Edouard-chin left a comment

Choose a reason for hiding this comment

Uh oh!

couimet commented Oct 24, 2025

Uh oh!

Uh oh!

Uh oh!

Edouard-chin commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelfranca commented Oct 24, 2025

Uh oh!

couimet commented Oct 28, 2025

Uh oh!

Edouard-chin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

couimet commented Oct 28, 2025

Uh oh!

Edouard-chin left a comment

Choose a reason for hiding this comment

Uh oh!

couimet commented Oct 29, 2025

Uh oh!

Uh oh!

Edouard-chin commented Oct 29, 2025

Uh oh!

couimet commented Oct 30, 2025

Uh oh!

couimet commented Oct 30, 2025

Uh oh!

Edouard-chin commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `context` attribute to link `TODO`s to GitHub issues #107

Add `context` attribute to link `TODO`s to GitHub issues #107

couimet commented Oct 23, 2025 •

edited

Loading

Edouard-chin commented Oct 24, 2025 •

edited

Loading